-
Notifications
You must be signed in to change notification settings - Fork 551
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
allow fetching file urls when experimental permissions are enabled #3775
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
I think there are a few rough edges to fix still, but this is fine currently.
Fetching file URLs is heavily requested and leads to things that work in the browser, Deno, and Bun, but not node ( What I am getting at: this is practically useless in its current state. Should I remove the requirement on the permission system? Or, should we use permissions if they're available, and allow fetching any file URL if they aren't? |
Developers put unsanitized URLs in fetch() all the time. This is security vs DX, and this time I don't think we can support DX.
We could if we added another option like I think this would be the best option. If so, print an experimental warning. |
My biggest issue is definitely the fourth point: having to explicitly enable every permission if you use any permissions. As in, to allow fetching file URLs, without changing your app's functionality, you need to do:
Then of course you might limit reading files, so I assume most people will |
Adding an option seems like a false sense of security to me. If any dependency/sub-dependency uses the option, the same vulnerabilities will exist as if we disregarded permissions. When the |
Note that if they only limit reading the current folder, or just a few files in it, this will be safe to use. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't need to enforce permission model here. It should be up to the user.
My suggestion is:
- Allow user to opt-in to the fetch file URLs through a config option (as Matteo suggested)
- If permission model is enabled we check if the user has access to the file request
- If don't, a custom message is exhibited (the current state) -
Access to ${fileURL.href} is not permitted
- If don't, a custom message is exhibited (the current state) -
I don't think a library should change its behavior based on permission model state. It should be transparent to users.
this enables fetching file urls when permissions are enabled and we have
fs.read
access to the file.